fix: zombie connection after stream-error 500 (ClientPayload + receipt compliance) - #630
Conversation
The core lib should stay agnostic of any specific consumer. Removes mentions of external repos/issue numbers from inline comments and test module docs.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughTighten ACK gating/docs for incoming messages, extract and normalize delivery-receipt node construction (adds context/participant/type), allow receipts for status broadcasts, persist a login-counter increment post-login for paired devices, make WA Web payload omit auto-generated phone_id and set login pull=true, plus tests and minor docs/dependency cleanup. ChangesMessage Ack and Receipt Gating
Sequence Diagram(s)sequenceDiagram
participant Client as Client (incoming handler)
participant ShouldAck as Client::should_ack
participant ReceiptPolicy as Client::should_send_delivery_receipt
participant Builder as build_delivery_receipt_node
participant Sender as Client::send_delivery_receipt
Client->>ShouldAck: ask whether to ACK incoming <message/>
ShouldAck-->>Client: true/false (newsletter -> ack class="message")
Client->>ReceiptPolicy: check delivery receipt eligibility
ReceiptPolicy-->>Client: allow/deny (reject when id empty or newsletter)
ReceiptPolicy->>Builder: request <receipt> node
Builder-->>Sender: constructed <receipt> (id,to,participant,type,context)
Sender-->>Client: dispatch receipt
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labelsapi-design 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/receipt.rs (1)
199-213:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the stale status-broadcast docstring.
Lines 206-207 still say delivery receipts are skipped for status broadcasts, but this path now allows them and emits
context="status". This is going to send the next person debugging receipts in the wrong direction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/receipt.rs` around lines 199 - 213, The docstring for send_delivery_receipt is outdated about status broadcasts; update the comment above send_delivery_receipt (and/or should_send_delivery_receipt) to reflect that delivery receipts are no longer skipped for status broadcasts but are emitted with context="status" instead of being ignored. Mention the behaviors covered (DMs, group messages, peer messages with type="peer_msg") and replace the line that says "It correctly skips sending receipts for status broadcasts" with a clear statement that status broadcasts now receive receipts with context="status" so future readers/debbuggers are not misled; keep references to send_delivery_receipt, should_send_delivery_receipt, and build_delivery_receipt_node for locating the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/receipt.rs`:
- Around line 199-213: The docstring for send_delivery_receipt is outdated about
status broadcasts; update the comment above send_delivery_receipt (and/or
should_send_delivery_receipt) to reflect that delivery receipts are no longer
skipped for status broadcasts but are emitted with context="status" instead of
being ignored. Mention the behaviors covered (DMs, group messages, peer messages
with type="peer_msg") and replace the line that says "It correctly skips sending
receipts for status broadcasts" with a clear statement that status broadcasts
now receive receipts with context="status" so future readers/debbuggers are not
misled; keep references to send_delivery_receipt, should_send_delivery_receipt,
and build_delivery_receipt_node for locating the logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 94d8908b-e30d-47ac-a176-bcb2032bc66b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
src/client.rssrc/flush_scope.rssrc/receipt.rssrc/send.rstests/e2e/tests/concurrent_disconnect.rswacore/Cargo.tomlwacore/src/store/device.rs
💤 Files with no reviewable changes (1)
- wacore/Cargo.toml
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12080cc57d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "message" => from | ||
| .to_jid() | ||
| .is_some_and(|j| j.is_newsletter() || j.is_status_broadcast()), | ||
| "message" => from.to_jid().is_some_and(|j| j.is_newsletter()), |
There was a problem hiding this comment.
Keep fallback ack for status messages without receipts
Removing status@broadcast from should_ack means status <message> stanzas are no longer auto-acked here, but status receipts are only sent on successful message processing (dispatch_parsed_message → send_delivery_receipt). In status drop/error paths (for example expired-status branches in src/message.rs that intentionally continue without retry), no delivery receipt is emitted, so these stanzas now get neither <ack> nor <receipt> and can be redelivered indefinitely. Please keep a fallback ack for status messages when no receipt is produced, or emit a receipt from those drop paths.
Useful? React with 👍 / 👎.
Three wire diffs vs WA Web's WAWebClientPayload (Client/Payload.js +
Get/ClientPayloadForLogin.js):
1. UserAgent.phoneId — auto-generated UUID per build. WA Web never sets
the field. Per-connect rotation gets flagged as a device-fingerprint
anomaly and the server silently kills the application-layer session
(Noise handshake still succeeds, downstream stanzas get dropped).
Now passed through from profile, default None → omitted on the wire.
2. ClientPayload.pull — was unset on login. WA Web's login wrapper
hardcodes `{passive: false, pull: true}`; only `passive` is dynamic
(via PassiveTaskManager). Now sends pull=true for logins,
pull=false for registration (matches both paths).
3. Cleanup: drop the now-unused `uuid` workspace dep from wacore
(libsignal subcrate keeps its own).
Adds regression tests:
- phone_id_default_is_omitted_and_payload_is_deterministic: default
None and repeated get_client_payload() calls produce identical
wire bytes
- phone_id_passes_through_from_profile_when_set: opt-in works
- login_payload_phone_id_is_omitted_by_default
- login_payload_pull_is_true / registration_payload_pull_is_false
- login_payload_lc_reflects_login_counter: validates lc read-path
even though the bump dispatch is added in a follow-up commit
- login_counter_survives_serde_roundtrip: persistence layer integrity
DeviceCommand::IncrementLoginCounter has been defined, persisted, and unit-tested since the audit commit but was never dispatched, so `lc` stayed at 0 across every login. WA Web bumps it after each successful auth (Start/Backend.js listener on `onOpenSocketStream`, fired by Comms `onConnect` post-handshake). A flat-zero counter is one of the server's anti-abuse signals — combined with the rotating phone_id fixed in the previous commit, the result was the zombie-session symptom. Dispatched from the existing post-success spawn in handle_success, right after the LID-update block. process_command() flushes synchronously so the next handshake reads the bumped value.
WA Web (Handle/MsgSendReceipt.js + Send/DeliveryReceiptJob.js) acks status@broadcast messages with `<receipt context="status">`, not <ack>. This port had it inverted: should_ack: status_broadcast => true (sent <ack>) send_delivery_receipt: status_broadcast => skipped So status messages were getting the wrong wire shape, and the delivery-receipt code path never ran for them. Fix: drop status_broadcast from should_ack (newsletter is the only <message> tag that needs <ack> on the success path), let send_delivery_receipt cover it, and add the `context="status"` attr when chat.is_status_broadcast(). The receipt node assembly is extracted to a pure build_delivery_receipt_node() helper so unit tests can assert wire shape without spinning a transport. Adds: - delivery_receipt_for_status_broadcast_carries_context_status - delivery_receipt_for_dm_has_no_context_no_participant - delivery_receipt_for_group_carries_participant - should_send_delivery_receipt_allows_status_broadcast - should_send_delivery_receipt_skips_newsletter Updates test_ack_behavior_for_incoming_stanzas to assert the new contract for status@broadcast.
12080cc to
5f3ccaf
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…back The previous receipt-compliance commit dropped status@broadcast from should_ack to match WA Web's send-path (which uses <receipt context= "status">, not <ack>, on success). But process_group_enc_batch has three branches that intentionally fall through with `continue` for status messages — expired status, NoSenderKeyState, and the generic decrypt-error fallback — and the existing comment there explicitly forbids emitting a delivery receipt for undecryptable messages (it would inflate the server-side offline counter for stanzas we'll never process). Without should_ack returning true, those drop paths emit neither <ack> nor <receipt>, leaving the server to redeliver indefinitely. Restore the gate as a transport-layer fallback. The success path also emits <receipt context="status">; the duplicate is tolerated by the server (different attribute namespaces).
WA Web's Send/DeliveryReceiptJob.js gates the participant attr on
`(t.isGroup() || t.isBroadcast()) && r`, so status broadcasts (which
are isBroadcast=true but isGroup=false) also carry the original
poster's JID. Without it the server can't map the ack back to the
status owner and may keep retrying delivery.
Updates build_delivery_receipt_node to extend the gate, and tightens
the unit test to assert both context="status" AND the participant.
Adds defensive tests covering:
- peer-msg DM (type=peer_msg, no participant/context)
- status broadcast that happens to be category=Peer
(participant + context must still be set)
- should_send_delivery_receipt edge cases: empty id, own DM,
own peer-msg
Comms/Config.js gates `triggerOpenSocketStream` (which fires the incrementLoginCounter listener registered in Start/Backend.js) on `WAWebUserPrefsMultiDevice.isRegistered()`. So WA Web does NOT bump the counter during the XX handshake of the pairing flow — only on subsequent IK logins. The previous commit dispatched unconditionally on every <success>, which would push the first post-pair login to `lc=1` instead of the expected `lc=0`. Counter is still monotonic so the server tolerates it, but it's a measurable wire diff vs the official client. Mirror the gate by reading `device.pn` from the persistence snapshot (set during pairing) and skipping the bump while it's None. Adds `registration_payload_does_not_carry_lc` so a future refactor of the registration path can't silently leak the field.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Problem
After a
<stream:error code="500">the client reconnects, gets a fresh<success>(Noise keypair is still valid), but the server has silentlyinvalidated the application-layer session — every subsequent stanza is
dropped, including sends. Process restarts don't recover; only deleting
the auth state and re-pairing does.
Root cause
Two wire-level anti-abuse signals were off vs WA Web:
UserAgent.phoneIdwas being auto-generated as a fresh UUID on everypayload build. WA Web (
Client/Payload.js) never sets the field; aper-connect rotating value gets flagged as a device fingerprint anomaly.
ClientPayload.lcwas hardcoded to 0 becauseDeviceCommand::IncrementLoginCounterwas defined and persisted butnever dispatched. WA Web bumps it after every successful auth
(
Start/Backend.js→ CommsonConnect).Changes
Causa-raiz
fix(payload): stop generatingphone_id, sendpull=trueon login,align
lid_db_migratedcomment with WA Web's behavior.fix(client): dispatchIncrementLoginCounterafter<success>,gated on
device.pn.is_some()so the XX pairing handshake doesn'tbump it (matches WA Web's
isRegistered()check inComms/Config.js).Receipt compliance
fix(receipt): ackstatus@broadcastwith<receipt context="status">including the
participantattr — WA Web'sSend/DeliveryReceiptJob.jsgates the participant on(isGroup || isBroadcast) && participant, not justisGroup.fix(client): keepstatus@broadcastinshould_ackas a fallbackfor
process_group_enc_batchdrop paths (expired status, missingsender key, generic decrypt error). Those branches deliberately skip
the delivery receipt to avoid inflating the server offline counter;
without a transport
<ack>, the server retransmits forever.Tests
New regression coverage:
get_client_payload()producesidentical wire bytes, no per-build randomness.
lcreflectslogin_counterand persists across serde roundtrips.pull=trueon login,pull=falseon registration,lcomittedfrom the registration payload.
broadcast with participant + context=status, peer-msg DM, peer-msg
status edge case).
should_send_delivery_receiptgate: empty id, own DM, own peer-msg.should_ackgate: DM/group return false, newsletter andstatus@broadcast return true.
cargo fmt --all,cargo clippy --all --testsclean. Local test rungreen: 1577 unit tests, E2E suite green in CI.